Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(app, api-client, react-api-client): add api-client method for protocol reanalysis #14878

Merged
merged 17 commits into from
Apr 12, 2024

Conversation

ncdiehl11
Copy link
Collaborator

@ncdiehl11 ncdiehl11 commented Apr 11, 2024

closes AUTH-118

Overview

On the ODD, we need to trigger protocol reanalysis via a new /protocols/{protocolId}/analyses endpoint post request. This analysis should occur when a user sends confirms values on an RTP-containing protocol on the ODD.

reference: robot server PR

Test Plan

  • push this branch backend and ODD code to a Flex
  • restart robot server
  • send an RTP-containing protocol to Flex
  • walk through protocol setup, selecting at least one custom parameter value
  • observe overriden parameters at ProtocolSetup
  • observe that rerunning utilizes the same parameter overrides

Changelog

  • create api-client method and wrapper hook to hit reanalysis endpoint for a protocol to trigger analysis with RTP overrides
  • send RTP overrides to protocol run setup

Review requests

protocol authorship devs

Risk assessment

medium

@ncdiehl11 ncdiehl11 requested review from koji and jerader April 11, 2024 18:56
@ncdiehl11 ncdiehl11 self-assigned this Apr 11, 2024
@ncdiehl11 ncdiehl11 marked this pull request as ready for review April 11, 2024 18:57
@ncdiehl11 ncdiehl11 requested a review from a team as a code owner April 11, 2024 18:57
@@ -75,6 +75,7 @@ export interface InputFieldProps {
| typeof TYPOGRAPHY.textAlignCenter
/** small or medium input field height, relevant only */
size?: 'medium' | 'small'
ref?: React.MutableRefObject<null>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mistake from merging in edge. I will fix

@@ -1,5 +1,5 @@
import * as React from 'react'
import { KeyboardReact as Keyboard } from 'react-simple-keyboard'
import Keyboard from 'react-simple-keyboard'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other keyboards use import { KeyboardReact as Keyboard } from 'react-simple-keyboard'

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above. reverting...

Comment on lines 100 to 104
const { createProtocolAnalysis } = useCreateProtocolAnalysisMutation(
{},
protocolId,
host
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change like below

export function useCreateProtocolAnalysisMutation(
  protocolId: string | null,
  hostOverride?: HostConfig | null,
  options: UseCreateProtocolAnalysisMutationOptions | undefined = {}
): UseCreateProtocolMutationResult {

We don't need to pass {}.

Suggested change
const { createProtocolAnalysis } = useCreateProtocolAnalysisMutation(
{},
protocolId,
host
)
const { createProtocolAnalysis } = useCreateProtocolAnalysisMutation(
protocolId,
host
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smart. fixing that.

HostConfig,
RunTimeParameterCreateData,
} from '@opentrons/api-client'
import { ProtocolAnalysisSummary } from '@opentrons/shared-data'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import { ProtocolAnalysisSummary } from '@opentrons/shared-data'
import type { ProtocolAnalysisSummary } from '@opentrons/shared-data'

Comment on lines 1 to 7
import {
UseMutationResult,
UseMutationOptions,
useMutation,
UseMutateFunction,
useQueryClient,
} from 'react-query'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import {
UseMutationResult,
UseMutationOptions,
useMutation,
UseMutateFunction,
useQueryClient,
} from 'react-query'
import {
useMutation,
useQueryClient,
} from 'react-query'
import type {
UseMutationResult,
UseMutationOptions,
UseMutateFunction,
} from 'react-query'

>

export function useCreateProtocolAnalysisMutation(
options: UseCreateProtocolAnalysisMutationOptions = {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please see the above comment

@ncdiehl11 ncdiehl11 requested a review from koji April 11, 2024 21:57

import type { ResponsePromise } from '../request'
import type { HostConfig } from '../types'
import type { ProtocolAnalysisSummary } from '@opentrons/shared-data'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be higher

@@ -62,10 +70,10 @@ describe('ProtocolSetupParameters', () => {
})
it('renders the other setting when boolean param is selected', () => {
render(props)
screen.getByText('Off')
expect(screen.getAllByText('On')).toHaveLength(3)
screen.debug()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be removed if there is no specific reason to keep this debug line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just an oversight, thanks

Comment on lines 2 to 9
import type {
UseMutationResult,
UseMutationOptions,
UseMutateFunction,
} from 'react-query'
import { createProtocolAnalysis } from '@opentrons/api-client'
import { useHost } from '../api'
import type { AxiosError } from 'axios'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import type {
UseMutationResult,
UseMutationOptions,
UseMutateFunction,
} from 'react-query'
import { createProtocolAnalysis } from '@opentrons/api-client'
import { useHost } from '../api'
import type { AxiosError } from 'axios'
import { createProtocolAnalysis } from '@opentrons/api-client'
import { useHost } from '../api'
import type { AxiosError } from 'axios'
import type {
UseMutationResult,
UseMutationOptions,
UseMutateFunction,
} from 'react-query'

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will reorder according to this pattern

response.data
)
)
.catch(e => {
Copy link
Contributor

@koji koji Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
.catch(e => {
.catch((e: Error) => {

})
return response.data
})
.catch(e => {
Copy link
Contributor

@koji koji Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
.catch(e => {
.catch((e: Error) => {

Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a few comments but the changes look good.
I tested this branch on a Flex and this worked as expected.

@ncdiehl11 ncdiehl11 merged commit 485bf0e into edge Apr 12, 2024
22 checks passed
@ncdiehl11 ncdiehl11 deleted the feat_api-client-protocol-reanalysis branch April 12, 2024 17:19
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants